Skip to content

security: reject code-bearing snapshot fields (execute/domTransformation) from the local API (PER-8607, PER-8613)#2280

Open
Shivanshu-07 wants to merge 1 commit into
masterfrom
security/PER-8607-8613-block-remote-scripts
Open

security: reject code-bearing snapshot fields (execute/domTransformation) from the local API (PER-8607, PER-8613)#2280
Shivanshu-07 wants to merge 1 commit into
masterfrom
security/PER-8607-8613-block-remote-scripts

Conversation

@Shivanshu-07

@Shivanshu-07 Shivanshu-07 commented Jun 14, 2026 •
edited by atlassian Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

Third focused percy-cli security PR — two High-severity code-injection findings (CVSS 8.8, deadline 2026-06-16).

Ticket CWE Finding
PER-8607 CWE-94 domTransformation string from POST body reaches window.eval in Chromium
PER-8613 CWE-94 Attacker-controlled JS injected into Chromium via CDP (execute hooks)

Root cause

The local /percy/snapshot endpoint is unauthenticated. execute (run via page.eval) and domTransformation (passed to window.eval in @percy/dom) flow straight from req.body into the browser context of the page being snapshotted. Since Percy often snapshots authenticated/staging pages, an injected script can exfiltrate cookies/tokens or SSRF internal services.

Fix

Added stripRemoteScriptFields() and wired it into the POST /percy/snapshot handler: execute and domTransformation are removed (recursively, including additionalSnapshots[]) from the network request body before it reaches percy.snapshot(), with a warning logged.

Why this is safe for legitimate use: the config-file / CLI path (percy snapshot <url|sitemap>) calls percy.snapshot() directly and never passes through this HTTP route (confirmed in packages/cli-snapshot/src/snapshot.js → percy.yield.snapshot(options)), so config-sourced execute/domTransformation are unaffected. This mirrors the existing stripBlockedConfigFields control already applied to /percy/config.

Trusted programmatic callers that intentionally POST a URL + execute/domTransformation to the local API can opt back in with PERCY_ALLOW_REMOTE_SCRIPTS=true.

Verification

  • node --check passes; logic verified against the ticket's exploit payloads (top-level + nested execute/domTransformation removed; url/name/additionalSnapshots[].name preserved; original body not mutated; opt-in retains fields).
  • Added unit tests in api.test.js (describe('stripRemoteScriptFields')): removal, no-op + no-warn on clean bodies, opt-in override, non-object bodies.

⚠️ Behaviour change — flagged for review. Callers that POST a URL snapshot to the local API and rely on server-side execute/domTransformation will have those ignored unless PERCY_ALLOW_REMOTE_SCRIPTS=true. DOM-capturing SDKs (which POST a pre-serialized domSnapshot) are unaffected, as is the percy snapshot CLI command.

Closes PER-8607, PER-8613.

🤖 Generated with Claude Code

@Shivanshu-07 @claude
security: reject code-bearing snapshot fields from the local API (PER…
3706fa1 
…-8607, PER-8613)

The local /percy/snapshot endpoint is unauthenticated, so accepting
`execute` (run via page.eval) and `domTransformation` (passed to window.eval)
from a network request body lets any local caller inject arbitrary JavaScript
into the — possibly authenticated — page being snapshotted (CWE-94, CVSS 8.8).

Add stripRemoteScriptFields(): the POST /percy/snapshot handler now strips
`execute` and `domTransformation` (recursively, incl. additionalSnapshots[])
from the request body before it reaches percy.snapshot(), logging a warning
when it does. The config-file / CLI path (`percy snapshot`) calls
percy.snapshot() directly and never traverses this route, so legitimate
config-sourced execute/domTransformation continue to work. Trusted programmatic
callers can opt back in with PERCY_ALLOW_REMOTE_SCRIPTS=true.

Mirrors the existing stripBlockedConfigFields control on /percy/config. Adds
unit tests covering removal, nested removal, the opt-in override, and
non-object bodies.

NOTE: this changes default behaviour for any caller that POSTs a URL snapshot
with execute/domTransformation to the local API expecting server-side
execution — those fields are now ignored unless the opt-in env is set. Flagged
for review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Shivanshu-07 Shivanshu-07 requested a review from a team as a code owner June 14, 2026 16:43
@Shivanshu-07

Shivanshu-07 commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Claude Code PR Review

PR: #2280 • Head: 3706fa1 • Reviewers: stack:code-reviewer

Summary

Adds stripRemoteScriptFields() to packages/core/src/api.js, which deep-walks the /percy/snapshot request body and removes the code-bearing execute and domTransformation fields before they reach percy.snapshot() — closing a local-API code-injection vector (PER-8607, PER-8613). The strip is recursive (covers additionalSnapshots), non-mutating (deep clone), and gated by a PERCY_ALLOW_REMOTE_SCRIPTS=true escape hatch. Four unit tests cover the main scenarios.

Review Table

Priority Category Check Status Notes
High Security Input validation and sanitization Pass Code-bearing fields stripped before percy.snapshot()
High Security No code-injection bypass Pass Recursive walk covers nested additionalSnapshots; matched keys deleted before children walked
High Correctness Logic correct, edge cases Pass Non-mutating deep clone; strict === 'true' bypass check
High Correctness Error handling explicit Pass —
Medium Testing New code has tests Pass 4 targeted unit tests in api.test.js
Medium Quality Follows existing patterns Pass Mirrors stripBlockedConfigFields style
Low Quality Naming / no dead code Pass —

Findings

  • File: packages/core/src/api.js (stripRemoteScriptFields)

  • Severity: Low

  • Issue: Field-name matching is exact/case-sensitive (Execute, EXECUTE not stripped). Not exploitable today — Percy's downstream config/snapshot normalisation is also case-sensitive JS property access, so a mis-cased field never reaches an eval sink — but worth a toLowerCase() guard to future-proof against an upstream proxy/parser that case-folds.

  • Suggestion: Normalise key casing before the includes() check.

  • File: packages/core/src/api.js (createPercyServer) / docs

  • Severity: Low

  • Issue: PERCY_ALLOW_REMOTE_SCRIPTS=true is undocumented and has no access control. A customer enabling it from a forum workaround re-exposes the CWE-94 risk.

  • Suggestion: Document the variable with a security warning and emit a log.warn at server startup when it is set.

  • File: packages/core/test/api.test.js:1910

  • Severity: Low

  • Issue: The warn-message assertion stringMatching(/Ignoring \execute`, `domTransformation`/)` implicitly depends on V8 key-enumeration order.

  • Suggestion: Assert the two field names independently, or sort the removed set before joining.

Note (Low, pre-existing): /percy/flush, /percy/comparison, /percy/automateScreenshot aren't covered, but none of those paths execute snapshot DOM-capture scripts in the current architecture — no live vulnerability.

Verdict: PASS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@prklm10 prklm10 Awaiting requested review from prklm10 prklm10 is a code owner automatically assigned from percy/percy-product-reviewers

@pranavz28 pranavz28 Awaiting requested review from pranavz28 pranavz28 is a code owner automatically assigned from percy/percy-product-reviewers

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Shivanshu-07